Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: removes Readable.fromWeb and Readable.toWeb methods #506

Merged
merged 3 commits into from
May 8, 2023

Conversation

RishabhKodes
Copy link
Contributor

fixes: #482

The Readable.fromWeb and Readable.toWeb methods were no longer being used from here (instead being imported from require(stream') method).

This pr removes both methods during the build when the module is extracted from Node.js.

Signed-off-by: Rishabh Bhandari <rishabhbhandari6@gmail.com>
@mcollina
Copy link
Member

Can you move those to the replacements.mjs file?

@RishabhKodes
Copy link
Contributor Author

RishabhKodes commented Feb 26, 2023

Can you move those to the replacements.mjs file?

Do you mean moving the code changes I made, or the doing something like this https://github.com/nodejs/readable-stream/blob/main/build/build.mjs#L94 ?

@mcollina
Copy link
Member

Using the regular expressions instead.

Signed-off-by: Rishabh Bhandari <rishabhbhandari6@gmail.com>
@RishabhKodes
Copy link
Contributor Author

@mcollina I've moved the changes to replacements.mjs and put the code inside a function and then imported it in the build.mjs file, PTAL let me know if that's fine.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was trying to say is to use the system that's already in place and not add new code, just replacements via regexps.

Signed-off-by: Rishabh Bhandari <rishabhbhandari6@gmail.com>
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@RishabhKodes
Copy link
Contributor Author

@mcollina is the windows-latest, 18.x test flaky? I've seen it fail in a couple of other pull requests as well.

@RishabhKodes
Copy link
Contributor Author

@benjamingr @mcollina the Node.js / Node.js (windows-latest, 18.x) test is still failing after the re-run.

@RishabhKodes
Copy link
Contributor Author

@mcollina @benjamingr, an update on this pr? Has been idle for a long time.

@mcollina mcollina mentioned this pull request Mar 20, 2023
@mcollina mcollina merged commit 29fae5b into nodejs:main May 8, 2023
@mcollina
Copy link
Member

mcollina commented May 8, 2023

Sorry about it, it was buried down my github notifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readable.fromWeb and Readable.toWeb do not seems to be properly implemented
3 participants